Skip to content

fix(bpv7): the extension editor refuses what the parser rejects - #753

Open
ricktaylor wants to merge 2 commits into
mainfrom
fix/bpv7-extension-editor-accept-set
Open

ricktaylor wants to merge 2 commits into
mainfrom
fix/bpv7-extension-editor-accept-set

Conversation

@ricktaylor

@ricktaylor ricktaylor commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

Summary

ExtensionEditor could accept an edit at call time that then failed at finish(), or that produced bytes the bpv7 parser rejects. This PR makes the editor refuse those edits up front, with the parser's own error. It also fixes Builder::build, which could produce bundles its own parser rejects.

The gaps came out of the external review of #712. The BPA's filter editor mirrors ExtensionEditor, and a Rewriter that hits one of these gaps aborts the node under the Rewriter fail-stop rule, so they need closing in bpv7 first. #712 and the rest of the train are rebased onto this branch.

Changes

  • ExtensionEditor refusals. insert now refuses, as extension_editor::Error::Invalid carrying the error the parser raises for the same input:

    • a report_on_failure flag on an administrative-record or null-source bundle (RFC 9171 §4.2.3-4/-5): InvalidFlags;
    • an unrecognised CRC type: InvalidCrc;
    • a Previous Node, Bundle Age or Hop Count body that doesn't decode as its type: that type's decode error.

    replace also checks well-known bodies. There's one new variant wrapping crate::Error, rather than duplicates of errors the crate already has.

  • One statement of the RFC rule. PrimaryBlock::forbids_report_on_failure() is the single place RFC 9171 §4.2.3-4/-5 is written. The parser's block check and the editor's refusal both use it.

  • Builder::build fix. with_hop_count sets report_on_failure, so any null-source or admin-record bundle with a hop limit failed to parse. build() now clears the flag on every block of such a bundle, the same way it normalises the fragment flag.

  • Docs. The CHANGELOG has entries under Added (the refusals, the predicate) and Fixed (the builder). The TODO has a new entry for a fuzz target that pins the invariant: for every parseable bundle and every ExtensionEditor operation sequence, if finish() succeeds, the flattened result parses. The refusal list is kept in step with the parser by hand, so a future parser rule could reopen the gap.

API impact

ExtensionEditor is unreleased (listed under Unreleased → Added), so the new Error::Invalid variant breaks no released API. PrimaryBlock::forbids_report_on_failure() is a new public method. Callers of Builder that relied on the forbidden flag being emitted were producing bundles the parser rejects.

Tests

New tests in bpv7/tests/editor.rs and bpv7/tests/parse.rs:

  • the forbidden-flag refusal, for both an admin-record bundle and a null-source bundle;
  • the unrecognised-CRC refusal;
  • undecodable well-known bodies refused on insert and on replace;
  • a null-source / admin-record bundle built with a hop count clears report_on_failure and round-trips through parse.

Each refusal test asserts the specific wrapped parser error, not just is_err().

Verification

Run locally: cargo fmt --check, workspace cargo clippy --all-targets --all-features -- -D warnings, and cargo test --workspace --all-features. The only failures are environment-only: the storage-harness Postgres/S3 suites (no services here) and tcpclv4 [::1] (no IPv6). The no-std thumb build pair runs in CI only, and this PR needs it green.

Merge order

This merges first, then #712, #717, #718, #719 and #752. Until it merges, #712's diff also shows this commit.

🤖 Generated with Claude Code

@ricktaylor
ricktaylor force-pushed the fix/bpv7-extension-editor-accept-set branch 2 times, most recently from 19271ff to f398ff9 Compare October 2, 2026 17:28
ricktaylor and others added 2 commits October 2, 2026 20:41
`ExtensionEditor::insert` passed flags, CRC type and block bodies
through unchecked, so an edit could be accepted at call time and then
fail at `finish()` or produce a malformed bundle: a `report_on_failure`
flag on an administrative-record or null-source bundle (RFC 9171
§4.2.3-4/-5), an unrecognised CRC type, or a Previous Node / Bundle Age
/ Hop Count body that does not decode. Each is now refused at call
time: the parser's rejections as `Error::Invalid`, carrying its
`InvalidFlags` or `InvalidCrc`, and an undecodable body as
`Error::UndecodableBody`, carrying the type and the decode error a
receiving BPA raises, since the structural parser never decodes those
bodies. `replace` validates well-known bodies too. Receiver policy over
a well-formed bundle (Bundle Age on an unclocked bundle, hop limits)
stays the caller's decision.

The RFC rule gets one statement, `PrimaryBlock::forbids_report_on_failure`:
the parser's block check rejects through it, `Builder` clears the flag,
`ExtensionEditor` refuses it at call time, and the owner `Editor`
refuses it when it rebuilds, with the new
`editor::Error::ReportOnFailureForbidden` — there rather than at its
setters, since the primary can change after a block flag is set. The
bpa forwarder computes its per-hop flags from it.

`Builder::build` emitted bundles its own parser rejects through the
block flag: `with_hop_count` sets `report_on_failure`, so any
null-source or admin-record bundle with a hop limit was unparseable.
`build()` now clears the flag on every block of such a bundle, the
payload included, normalised like the fragment flag. The primary-level
half of the same clauses is ledgered in the TODO.

Tests pin each refusal with its accept control (an ordinary bundle
takes the flag, CRC16 and CRC32 inserts keep their type, a valid body
survives byte for byte, a replacement keeps its block's flags and CRC
type), the owner editor's refusal at rebuild whichever order the flag
and the forbidding primary were set in, the builder's normalisation per
block against literal expectations, and the parser's rejection of
spliced wire flags in null-source and admin-record bundles, with an
ordinary bundle carrying the flag through. The TODO records the
editor's accept-set invariant, the write doors' parse-only
`CrcType::Unrecognised`, and the builder's primary-level gap; two
broken doc links are fixed.

Found by the external review of #712: the BPA's filter editor mirrored
this one, and a Rewriter tripping the gap aborted the node.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Rick Taylor <rtaylor@aalyria.com>
`block::Flags`, `bundle::Flags` and `rfc9173::ScopeFlags` encode every
bit they hold, so a named bit carried in `unrecognised` reached the
wire as that flag while its field read false. It slipped past
`ExtensionEditor`'s `report_on_failure` refusal and `Builder::build`'s
normalisation into bytes the parser rejects, and in a BPSec scope it
left the primary block out of the IPPT/AAD the security source computed
while the emitted parameter told the receiver to include it.

All three gain `canonicalize()`, which folds each alias bit into its
named field, after `block::Type::canonicalize()`; `ScopeFlags` gains the
`From<u64>`/`From<&ScopeFlags> for u64` pair its codec now delegates to.
Every method that takes flags canonicalizes them — `Builder::with_flags`,
both `BlockBuilder::with_flags`s, `Editor::with_bundle_flags`,
`ExtensionEditor::insert`, and the BIB-HMAC-SHA2 and BCB-AES-GCM
operations' scope, with `BlockTemplate::new` as the backstop — so an
alias is refused, normalised or covered as the flag it encodes, and a
returned `Bundle` holds what its bytes hold. The IPPT and AAD builders
keep zeroing unassigned scope bits, as RFC 9173 §3.7 and §4.7.2 require,
and now say so.

Equality and hashing compare the encoding, on the three flag types and
on `block::Type`, whose ordering becomes the code order: an alias equals
its canonical form, so an aliased security scope shares one BIB with
its canonical form, and `is_canonical()` is the test that tells them
apart. Serde canonicalizes `block::Flags`, `bundle::Flags` and
`block::Type` in both directions through same-shape mirrors, so the
stored form is unchanged and always canonical; direct field writes are
not canonicalized, and the type docs say so.

BREAKING: `unrecognised` is a plain `u64` on all three types, zero when
no unrecognised bits are set (was `Option<u64>`, where `None` and
`Some(0)` meant the same): `None` becomes `0` and `Some(bits)` becomes
`bits`. Serde omits a zero field, so the shape is unchanged for every
value a parse or builder produces, except that an explicit `null` no
longer deserializes. The bpv7 tool's `inspect` and the bpa validity
filter's log follow. `block::Flags` and `bundle::Flags` implement
`Hash`, as `ScopeFlags` does; equality, hashing and `block::Type`'s
ordering go by the encoding.

Tests walk all 64 bits of each type against the RFCs' bit assignments
(RFC 9171 §4.2.3 and §4.2.4, RFC 9173 §3.3.3), each named bit decoding
to the field the RFC names it — wire round-trip, alias encoding,
canonicalization — and pin each door: the builder makes an
administrative record of the admin-bit alias and normalises its blocks,
a rebuilt or built `Bundle` matches its bytes, the extension editor
refuses the forbidden flag's alias, signing and encryption under an
aliased scope emit the folded scope and verify and decrypt, an aliased
and a canonical scope share one BIB, and an inline test pins the
`BlockTemplate::new` backstop. Every test that proves canonical form
asserts `is_canonical()` beside `==`; serde canonicalizes both ways and
refuses `null`; block types compare and order by code. The block and
bundle walks live in tests/flags.rs and the builder tests in
tests/builder.rs. `bundle inspect` lists a non-fragment bundle's flags,
which sat inside the fragment check.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@ricktaylor
ricktaylor force-pushed the fix/bpv7-extension-editor-accept-set branch from f398ff9 to 1719f88 Compare October 2, 2026 20:42

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant